🚅 Deployed to the rivet-pr-4148 environment in rivet-frontend

• fix(pb, gas): fix overreporting slots bug, metrics wf not stopping bug, corrupt history preventing wf pull #4148 👈 (View in Graphite)
• main

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

This stack of pull requests is managed by Graphite. Learn more about stacking.

PR Review: Fix overreporting slots bug, metrics workflow not stopping bug, corrupt history preventing workflow pull

Summary

This PR addresses three critical bugs in the Pegboard and Gasoline packages:

1. Overreporting slots bug - Fixed serverless slot accounting
2. Metrics workflow not stopping - Added destroy signal when actor is destroyed early
3. Corrupt history preventing workflow pull - Changed error handling to gracefully skip corrupted workflows instead of failing

Code Quality & Best Practices

✅ Strengths

• Error handling improvement: Converting ensure! to graceful error logging in gasoline/src/db/kv/mod.rs is a pragmatic approach that prevents workflow system deadlock due to corrupt history
• Clear comments: The new comment in runtime.rs:1115-1117 clearly explains the reasoning behind conditional slot clearing
• Structured logging: Using tracing::error!(?wf, ...) follows the structured logging pattern from CLAUDE.md
• Improved error messages: More specific error messages ("indexed name", "indexed chunk") help with debugging

⚠️ Concerns & Suggestions

1. Slot Accounting Logic (runtime.rs:1135-1138)

// Only mark allocated_serverless_slot as false if it was allocated before and cleared now
if allocated_serverless_slot && cleared {
    state.allocated_serverless_slot = false;
}

Issue: The previous logic unconditionally set allocated_serverless_slot = false, which could have been causing the slot overreporting. This fix is correct, but the logic is subtle.

Consideration: Should there be logging when we skip clearing the slot? This would help diagnose future slot accounting issues:

if allocated_serverless_slot && cleared {
    state.allocated_serverless_slot = false;
} else if allocated_serverless_slot && !cleared {
    tracing::debug!(actor_id=?input.actor_id, "pending allocation already cleared, keeping slot allocation");
}

2. Silent Data Corruption Handling (kv/mod.rs:1635-1640)

if current_event.indexed_names.len() != key.index {
    tracing::error!(?wf, "corrupt history, indexed name doesn't exist yet or is out of order");
    return Ok(None);
}

Issue: While returning Ok(None) prevents system failure, it silently discards the entire workflow pull attempt. This could lead to workflows getting stuck.

Recommendations:

1. Add metrics/alerting for corrupt history detection
2. Consider including ?current_event and key_index=?key.index in the error log for better debugging
3. Document the recovery path - does returning None here cause a retry, or is the workflow permanently blocked?

Example improvement:

if current_event.indexed_names.len() != key.index {
    tracing::error!(
        ?wf,
        expected_index = key.index,
        actual_len = current_event.indexed_names.len(),
        ?current_event,
        "corrupt history: indexed name doesn't exist yet or is out of order"
    );
    // TODO: Add metric for corrupt history detection
    return Ok(None);
}

3. Metrics Workflow Signal (mod.rs:329-335)

runtime::SpawnActorOutput::Destroy => {
    ctx.v(2)
        .signal(metrics::Destroy { ts: util::timestamp::now() })
        .to_workflow_id(metrics_workflow_id)
        .send()
        .await?;

Question: Is there a risk that metrics_workflow_id might not exist or be invalid when the actor is destroyed early? Should there be error handling for the signal send?

4. Test Runner Environment Variables (test-runner/src/index.ts)
The changes to environment variable handling are improvements, but:

• parseInt(process.env.RIVET_RUNNER_TOTAL_SLOTS ?? "1") is good, but consider using a more realistic default than 1 slot
• String comparison == "1" works but is fragile. Consider: process.env.AUTOSTART_SERVER !== "0" for boolean flags
• Route changes from /start → /api/rivet/start look like API versioning. Ensure clients are updated accordingly.

5. Log Level Change (runtime.rs:519)

-tracing::warn!(actor_id=?input.actor_id, "failed to allocate (no availability), waiting for allocation");
+tracing::debug!(actor_id=?input.actor_id, "failed to allocate (no availability), waiting for allocation");

Question: Is this expected to happen frequently during normal operation? If so, debug is appropriate. If it indicates a capacity problem, warn might be better for alerting.

Potential Bugs

1. Workflow Recovery Path Unclear: When corrupt history is detected and Ok(None) is returned, what happens to the workflow? Is it retried? Does it need manual intervention?

2. Missing Error Handling: The signal send in mod.rs:334 uses ? to propagate errors, which will fail the entire actor workflow if the metrics workflow doesn't exist or can't receive signals.

Performance Considerations

No significant performance concerns. The changes are mostly bug fixes with minimal performance impact. The graceful handling of corrupt history actually improves system resilience.

Security Concerns

No security issues identified. The changes are focused on internal workflow state management.

Test Coverage

Missing Tests:

1. Test for slot accounting bug - verify that allocated_serverless_slot is correctly maintained when clear_pending_allocation is called with various states
2. Test for corrupt history handling - verify system continues operating when history corruption is detected
3. Test for metrics destroy signal - verify signal is sent when actor is destroyed early

Recommendation: Add unit tests for the slot accounting logic and integration tests for the corrupt history scenario.

Additional Notes

1. Documentation: Consider updating documentation about:

   • Recovery procedures for corrupt workflow history
   • Slot accounting semantics
   • Metrics workflow lifecycle

2. Follow-up Tasks:

   • Add monitoring/alerting for corrupt history detection
   • Investigate root cause of history corruption
   • Audit other uses of ensure! that might benefit from similar graceful handling

3. PR Description: The PR template wasn't filled out. Please update with:

   • Description of each bug and root cause
   • Testing performed to verify fixes
   • Migration/rollout considerations

Verdict

Approve with minor suggestions ✅

The fixes address real production issues and the approach is sound. The main concern is the silent failure on corrupt history - ensure there's visibility into when this happens and a clear recovery path.

Review generated by Claude Code - focusing on code quality, potential bugs, and maintainability

Merge activity

• Feb 6, 10:59 PM UTC: MasterPtato added this pull request to the Graphite merge queue.
• Feb 6, 11:00 PM UTC: CI is running for this pull request on a draft pull request (#4150) due to your merge queue CI optimization settings.
• Feb 6, 11:01 PM UTC: Merged by the Graphite merge queue via draft PR: #4150.